Manage edit documentation #8169
Conversation
lyndaidaii
requested review from
agr,
cristinamanum,
dannyjdev,
drewgillies,
joelverhagen,
loic-sharma,
ryuyu,
shishirx34,
skofman1,
xavierdecoster and
zhhyu
as code owners
src/NuGetGallery/Helpers/ViewModelExtensions/ManagePackageViewModelFactory.cs
Outdated
Show resolved
Hide resolved
| @@ -1,4 +1,5 @@ | |||
| <form aria-label="Edit your package" id="edit-readme-form"> | |||
| | |||
There was a problem hiding this comment.
remove extra line here
|
Please ignore previous comments. This is new different UI behavior comparing previous one |
| return; | ||
| } | ||
|
|
||
| if (model.Versions[selectedVersion].HasEmbeddedReadme) { |
There was a problem hiding this comment.
It hide edit/submit button when there is embeddedReadme and embeddedReadme feature flag is on. I am considering to remove the feature flag, looks like it's not necessary when roll back case.
For case like we turn off embedded feature, but there are legacy and embedded readme, we still don't allow user to edit embedded readme.
There was a problem hiding this comment.
I agree. There is no need to use the feature flag here. The feature flag is used to block customers to upload a package with an embedded readme file. Here checking the DB flag is enough.
| /// Signifies whether or not the embedded ReadMe exists | ||
| /// </summary> | ||
| [NotMapped] | ||
| public bool HasEmbeddedReadMe |
There was a problem hiding this comment.
I know our codebase is inconsistent between ReadMe and Readme, but from my understanding we would like to migrate to the Readme casing. If so, should we use the Readme casing on all new properties/methods you introduce?
| public bool HasEmbeddedReadMe | |
| public bool HasEmbeddedReadme |
| var result = new ManagePackageViewModel.VersionReadMeState( | ||
| submitUrlTemplate.Resolve(model), | ||
| getReadMeUrlTemplate.Resolve(model), | ||
| null); |
There was a problem hiding this comment.
null [](start = 16, length = 4)
use named params for readability
| @@ -156,6 +156,18 @@ public bool HasReadMe | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Signifies whether or not the embedded ReadMe exists | |||
There was a problem hiding this comment.
| /// Signifies whether or not the embedded ReadMe exists | |
| /// Signifies whether or not the embedded Readme exists |
| @@ -184,6 +185,22 @@ | |||
| if (model === null || !model.IsSymbolsPackage) { | |||
| BindReadMeDataManager.bindReadMeData(model); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
remove extra line here
| @@ -38,6 +38,7 @@ public class DisplayPackageViewModel : ListPackageItemViewModel | |||
| public bool IsPackageDependentsEnabled { get; set; } | |||
| public NuGetPackageGitHubInformation GitHubDependenciesInformation { get; set; } | |||
| public bool HasEmbeddedIcon { get; set; } | |||
| public bool HasEmbeddedReadmeFile { get; set; } | |||
There was a problem hiding this comment.
Do we use this property anywhere?
There was a problem hiding this comment.
Good catch, it needs to be deleted
| @@ -62,7 +62,7 @@ | |||
| </div> | |||
| </div> | |||
|
|
|||
| @Html.Partial("_EditForm") | |||
| @Html.Partial("_EditForm", Model) | |||
| </div> | |||
There was a problem hiding this comment.
Do we need to pass the model here? I see that we have passed the model:
| @@ -50,6 +50,7 @@ public VersionReadMeState(string submitUrl, string getReadMeUrl, string readMe) | |||
| public string SubmitUrl { get; } | |||
| public string GetReadMeUrl { get; } | |||
| public string ReadMe { get; } | |||
| public bool HasEmbeddedReadme { get; set; } | |||
There was a problem hiding this comment.
Do you think whether it's better if we also set this value in the constructor, and restrict it to a read-only field? I see that we only use this field in the corresponding JS file, so it should be safe to do so?
|
Let's verify the behavior manually, as we don't have a good test coverage on views. |
Summary of the changes:
Documentation UI change in manage package page:
Question for discussion:
Feature flag looks like could be removed here.
For case like we turn off embedded feature, there are legacy and embedded readme, we still don't allow user to edit embedded readme.
Addresses https://github.com/NuGet/Engineering/issues/3307